Skip to content

Add GitHub Enterprise support#3720

Open
arhowe00 wants to merge 3 commits into
Kitware:masterfrom
arhowe00:add-ghe-support
Open

Add GitHub Enterprise support#3720
arhowe00 wants to merge 3 commits into
Kitware:masterfrom
arhowe00:add-ghe-support

Conversation

@arhowe00
Copy link
Copy Markdown

@arhowe00 arhowe00 commented May 13, 2026

This change makes it easier to configure GitHub integration for users who may use a private GHE instance. CDash's GitHub integration (check runs, commit statuses, PR comments) is currently hardcoded to api.github.com.

Could the reviewer look at (1) how I change GitHubClient as well as (2) whether these changes are reasonable?

(1) You can see that constructing with null as the api version sets it to 'v3' which is the version that the only enterprise client I'm aware of uses. Why is it currently 'machine-man-preview'?
(2) I changed this so checks are always posted if there is a revision. It seems this flag is now read-only. I've added another commit to guard createOrUpdateCheck, since if ($revision !== '') fires for any project regardless of whether it's actually configured with GHE

@williamjallen williamjallen added this to the v5.1 milestone May 14, 2026
@williamjallen williamjallen self-requested a review June 1, 2026 12:47
Enable CDash to post check runs and commit statuses to ghe installations
in addition to github.com.

Changes:
- Add GITHUB_ENTERPRISE_URL config option (env var)
- Pass enterprise URL to GitHubClient constructor with apiVersion=null
  (so it defaults to 'v3') so the PathPrepend plugin builds correct
  /api/v3/ paths for ghe
- Add isGitHubUrl() helpers in GitHub.php and RepositoryUtils.php to
  match repository URLs against both github.com and the configured GHE
  host
- Convert get_github_api_url() to construct /api/v3/repos/ paths for GHE
  repositories
- Make DoneHandler always create/update the check when a revision exists,
  rather than requiring the pendingSubmissions recheck flag
$builder = new GitHubBuilder();
$apiClient = new GitHubClient($builder, 'machine-man-preview');
$enterpriseUrl = config('cdash.github_enterprise_url');
$apiClient = new GitHubClient($builder, null, is_string($enterpriseUrl) ? $enterpriseUrl : null);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should treat an empty string like a null here.

Comment on lines +671 to +675
$enterpriseUrl = config('cdash.github_enterprise_url');
if (is_string($enterpriseUrl)) {
$host = parse_url($enterpriseUrl, PHP_URL_HOST);
return is_string($host) && str_contains($url, $host);
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're referencing the config value multiple times in this file. It would be better to factor that out into a method which returns the enterprise URL or null.

Comment on lines -87 to +91
$repositoryInterface = self::getRepositoryInterface($project);
try {
$repositoryInterface = self::getRepositoryInterface($project);
} catch (Exception $e) {
return;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? This will destroy all exceptions within getRepositoryInterface(), preventing them from making it to the logs.

Comment on lines -66 to +68
// Should we re-run any checks that were previously marked
// as pending?
if ($pendingSubmissionsModel !== null && $pendingSubmissionsModel->recheck) {
$revision = \App\Models\Build::findOrFail((int) $this->Build->Id)->updateStep->revision ?? '';
// Create or update the GitHub check for this commit.
$revision = \App\Models\Build::findOrFail((int) $this->Build->Id)->updateStep->revision ?? '';
if ($revision !== '') {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the thought process here?

Comment on lines +17 to +28
private static function isGitHubUrl(string $url): bool
{
if (str_contains($url, 'github.com')) {
return true;
}
$enterpriseUrl = config('cdash.github_enterprise_url');
if (is_string($enterpriseUrl)) {
$host = parse_url($enterpriseUrl, PHP_URL_HOST);
return is_string($host) && str_contains($url, $host);
}
return false;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a duplicate of isGitHubUrl() in app/cdash/app/Lib/Repository/GitHub.php. Let's remove the one in app/cdash/app/Lib/Repository/GitHub.php.

}
$enterpriseUrl = config('cdash.github_enterprise_url');
if (is_string($enterpriseUrl)) {
$host = parse_url($enterpriseUrl, PHP_URL_HOST);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Laravel's Uri facade to do this more eloquently (note: untested).

Apply throughout.

Suggested change
$host = parse_url($enterpriseUrl, PHP_URL_HOST);
$host = Uri::of($enterpriseUrl)->host();

Comment on lines +162 to +175
$enterpriseUrl = config('cdash.github_enterprise_url');
if (is_string($enterpriseUrl)) {
$host = parse_url($enterpriseUrl, PHP_URL_HOST);
if (is_string($host)) {
$idx = strpos($github_url, $host);
if ($idx !== false) {
// For GHE, ...://<host>/<user>/<repo> becomes ...://<host>/api/v3/repos/<user>/<repo>
$idx2 = $idx + strlen($host) + 1;
$api_url = substr($github_url, 0, $idx) . $host . '/api/v3/repos/';
$api_url .= substr($github_url, $idx2);
return $api_url;
}
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions/comments:

  1. Can you please add a comment explaining what's going on here?
  2. Does ...<host>/api/v3... work for GitHub.com too? i.e., are we strictly required to have a GHE-specific code path here?
  3. Assuming we need a GHE-specific code path, can you wrap the following code in an else so it's clear that there are two branches? It took a second for me to understand what was going on here.
  4. There are probably Laravel string facade methods which could make this cleaner.

@williamjallen williamjallen changed the title Add ghe support Add GitHub Enterprise support Jun 2, 2026
@williamjallen
Copy link
Copy Markdown
Collaborator

@arhowe00 Note that CDash squashes commits, so your PR description will be included in the git history permanently. Please leave any questions/comments as comments on GitHub and treat the PR description as the commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants